Skip to content

feat(gov-proposals): governance proposal creation (PR 8 backend)#8

Merged
sidhujag merged 21 commits intomainfrom
proposal-creation-pr8
Apr 22, 2026
Merged

feat(gov-proposals): governance proposal creation (PR 8 backend)#8
sidhujag merged 21 commits intomainfrom
proposal-creation-pr8

Conversation

@sidhujag
Copy link
Copy Markdown
Member

Summary

Backend slice for governance-proposal creation. Users can draft a
proposal, commit to its canonical hash, pay the 150 SYS collateral
(burned, non-refundable — enforced at the Core protocol level), and
have the backend auto-submit the gobject once the collateral clears 6
confirmations. All state changes surface via existing email channels
with full compliance footers.

Frontend wiring (Pali dApp interaction, PSBT builder, wizard, status
page, drafts list) lands in a sibling PR against sysnode-info.

What's new

Canonical hashing (lib/proposalHash.js)

JS port of CGovernanceObject::GetHash() from Syscoin Core. Lets the
dApp hand the OP_RETURN bytes to the user's own wallet instead of
calling gobject_prepare server-side (which would unconditionally
burn 150 SYS from the backend wallet on every attempt — not an option
for a wallet-less backend). Verified against frozen golden vectors +
field-sensitivity tests.

Canonicalization (lib/proposalValidate.js)

  • Emits exactly the on-chain fields (type/name/start_epoch/end_epoch/
    payment_address/payment_amount/url) with a fixed key order and
    non-scientific decimal amounts so the hash is byte-deterministic.
  • Stricter on URLs than Core (requires http(s)://) as a
    social-engineering guardrail; Core's CheckURL is permissive.
  • payment_count is deliberately NOT in canonical JSON — it is not a
    consensus field in Core; kept as display metadata only, with a soft
    1..60 bound in the route layer.

State machine (lib/proposalSubmissions.js + proposalDispatcher.js)

`prepared → awaiting_collateral → (submitted | failed)`. Repo enforces
the transition and raises stable `.code` errors. Dispatcher polls
`awaiting_collateral` rows once a minute, bumps confirmation counts
from `getRawTransaction`, and calls `gObject_submit` once
`>= GOVERNANCE_FEE_CONFIRMATIONS (6)`. Rejection vs transient error
classification is tested against Core's actual error strings.

HTTP surface (routes/govProposals.js → /gov/proposals)

  • Drafts CRUD with a 50-per-user soft cap
  • `POST /prepare` — canonicalize, compute hash, optional
    `gObject_check` pre-flight, persist `prepared` row (idempotent on
    `(userId, proposalHash)`), atomically consume source draft
  • `POST /submissions/:id/attach-collateral` — hand-off to the
    dispatcher
  • `GET/DELETE` for polling + cleanup

Mailer (lib/mailer.js)

  • `proposalSubmitted` — the "it's live on chain" email, surfaces
    both collateral txid and governance hash.
  • `proposalFailed` — covers both failure paths; explicit about the
    150 SYS burn.
  • New `proposal_notification` footer kind (transparency: you're
    getting this because you authored the proposal; cannot disable).

Design notes worth calling out

  • We auto-submit at 6 confs rather than letting Core handle immature
    collateral via its "postponed" path. The difference: one
    deterministic state transition observable by the user, instead of
    a silent Core-internal retry schedule we can't introspect.
  • `payment_count` exists as display metadata but is excluded from
    the hashed payload. Number of payouts is already implicit from
    `(end_epoch - start_epoch) / superblock_cycle`; adding it to the
    JSON would diverge from Core's canonical representation.
  • Dispatcher hooks (`onSubmitted`, `onFailed`) are the ONLY
    write-through the mailer has to state changes. Mailer failures are
    swallowed + logged so a broken SMTP relay never stalls the
    advance loop.

Test plan

  • Unit tests: hash (golden vectors), validate (canonical JSON,
    field stretching), repos (state machine), dispatcher (conf
    tracking, submit path, failure paths, hook wiring)
  • Route tests: full HTTP matrix including CSRF, auth, ownership,
    draft limits, idempotent prepare, atomic draft consumption,
    duplicate-txid detection, soft handling of transient RPC
  • Mailer tests: new templates + footer; XSS regression guards
  • Full suite: 673 / 673 passing, 3 consecutive clean runs

Follow-ups (explicitly out of scope for this PR)

  • Frontend wizard + Pali dApp interaction + manual-payment fallback
  • Drafts list UI on the governance page
  • PR 8 frontend ships against sysnode-info

🤖 Generated with Claude Code

Made with Cursor

jagdeep sidhu added 2 commits April 21, 2026 15:36
… step 1/3)

First slice of the governance proposal creation feature. Pure backend,
no wiring yet — appFactory + mailer template land in the next commit so
CI can exercise these modules in isolation first.

Summary:

- `lib/proposalHash.js` — JS port of Syscoin Core's
  `CGovernanceObject::GetHash()`. We need this because Core only
  exposes the hash via `gobject_prepare`, which ALWAYS burns 150 SYS
  from the node wallet. Computing it locally lets the dApp hand the
  OP_RETURN bytes to the user's own wallet (Pali or Syscoin-Qt) and
  keep the backend wallet-less. Verified against a frozen golden
  vector plus sensitivity tests.

- `lib/proposalValidate.js` — canonical JSON serializer + structural
  validator. Emits exactly the on-chain fields (type, name,
  start_epoch, end_epoch, payment_address, payment_amount, url) with
  a fixed key order and non-scientific decimal amounts so the hash
  is byte-deterministic. Stricter than Core on URL scheme
  (requires http(s)://) as a social-engineering guardrail.

- `db/migrations/001_init.sql` (in-place) — `proposal_drafts` and
  `proposal_submissions` tables. Submissions snapshot the canonical
  hashing fields (parent_hash, revision, time_unix, data_hex,
  proposal_hash) as immutable; the collateral OP_RETURN commits to
  these, so freezing them at create() time prevents drift.

- `lib/proposalDrafts.js` + tests — plaintext per-user drafts,
  BigInt-safe amounts (.safeIntegers(true)).

- `lib/proposalSubmissions.js` + tests — state machine
  (prepared → awaiting_collateral → submitted | failed) enforced
  inside the repo with stable `.code` errors.

- `lib/proposalDispatcher.js` + tests — background advance loop:
  polls `getRawTransaction`, bumps conf counts, fires
  `gObject_submit` once >= GOVERNANCE_FEE_CONFIRMATIONS (6) confs.
  Classifies Core errors as terminal (→ failed) vs transient (retry)
  and times out rows whose collateral is missing > 7 days.

- `routes/govProposals.js` + tests — HTTP surface:
  * drafts CRUD (50-per-user soft cap)
  * POST /prepare — canonicalize, compute hash, optional
    gObject_check pre-flight, persist `prepared` row (idempotent on
    (userId, proposalHash)), consume source draft in an atomic block
  * POST /submissions/:id/attach-collateral — hand-off to dispatcher
  * GET/DELETE for status polling and cleanup

Notes:

- `payment_count` is NOT a consensus field (Core has no bound on it;
  number of payouts is implicit from (end_epoch - start_epoch) /
  superblock_cycle). We keep it as pure display metadata with a soft
  1..60 range check in the route layer — never in canonical JSON.

- We submit after 6 confs rather than relying on Core's
  "postponed on immature collateral" path because it gives a cleaner
  UX: one deterministic state transition instead of a silent
  Core-internal retry schedule.

Tests: 658/658 passing, 5 consecutive clean full-suite runs.
Made-with: Cursor
Glues the governance-proposals subsystem into the running app and
closes the feedback loop with the user via email.

Mailer (lib/mailer.js):
  - New footer kind 'proposal_notification' — transparency copy
    specific to user-authored proposals (cannot disable, because
    the user themselves initiated each one by paying collateral).
  - New template `proposalSubmitted` — fired when a proposal reaches
    6 confs and Core returns a governance hash. Surfaces both the
    collateral txid and the governance hash so the user can verify
    on any block explorer without trusting us.
  - New template `proposalFailed` — fired on terminal failure
    (collateral_not_found after 7 days OR submit_rejected from
    Core). Body is explicit that the 150 SYS collateral is burned
    by protocol and cannot be refunded.
  - Both templates HTML-escape user-controlled fields (proposal
    name, fail detail) — regression-tested against <script> and
    onerror XSS payloads.

Dispatcher (lib/proposalDispatcher.js):
  - New optional `onSubmitted` / `onFailed` hooks fired after
    each real state transition, receiving the freshly re-read
    submission row.
  - Hook errors are caught + logged; they never propagate back
    into the advance loop (a failing mailer must not stall the
    state machine).
  - Hooks are `await`'d so async mailers are safe.

Wiring (lib/appFactory.js + server.js):
  - buildServices now constructs proposalDrafts + proposalSubmissions
    repos.
  - mountAuthAndVault accepts `proposalRpc` and — when provided —
    mounts /gov/proposals. Mount is unconditional on the rpc
    (drafts work offline) but the prepare pre-flight only runs
    when rpc.gObjectCheck is available.
  - createApp and server.js both build the dispatcher, wire it
    to the mailer via user-email lookup, and start a minute-level
    tick (first run staggered 5min after boot to avoid tramping
    the RPC node alongside the hourly reminder dispatcher).
  - server.js defines the camelCase→snake_case RPC adapter so
    only one module has to know about @syscoin/syscoin-js's
    `.call()` stub convention. Pre-existing /gov and
    reminderDispatcher wiring is untouched.

Tests: - Mailer: 9 new cases (new footer kind, new templates, HTML
    escape, detail-row conditional, burn disclosure on fail).
  - Dispatcher: 8 new cases covering the hooks (fire-once on
    submit, fire on both failure paths, async await, error
    swallowing, factory validation).
  - Full suite: 673 tests, stable across 3 consecutive runs.
Made-with: Cursor
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b089dc3bd9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread routes/govProposals.js Outdated
Comment on lines +497 to +500
if (typeof rpc.gObjectCheck === 'function') {
try {
const resp = await rpc.gObjectCheck(canon.dataHex);
const result =
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Call gObjectCheck with canonical proposal arguments

In routes/govProposals.js this preflight calls rpc.gObjectCheck with only dataHex, but the production adapter in server.js is defined as gObjectCheck(parentHash, revision, time, dataHex). With the current call shape, dataHex is passed as parentHash and the real payload becomes undefined, which causes invalid-params RPC failures; those failures match the /invalid/ classifier and can incorrectly return 422 core_rejected for otherwise valid proposals whenever RPC preflight is enabled.

Useful? React with 👍 / 👎.

Comment thread lib/proposalDispatcher.js Outdated
const TERMINAL_CORE_ERRORS = [
/rate limit/i,
/invalid/i,
/already exists/i,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Remove "already exists" from terminal submit failures

Treating /already exists/ as a terminal rejection can flip a genuinely published proposal to failed: if gObject_submit succeeded previously (e.g., process crash before markSubmitted, or overlapping dispatcher workers), a retry may return an "already exists" error even though the object is live on-chain. The current logic then calls markFailed(...), which can persist a false failure state and trigger incorrect failure notifications.

Useful? React with 👍 / 👎.

…bmit)

Two review findings from Codex on PR #8 (backend):

P1 (routes/govProposals.js): the /prepare pre-flight called
rpc.gObjectCheck with only dataHex, but server.js defines the
adapter with the full Core signature
(parentHash, revision, time, dataHex). That shifted args so dataHex
became parentHash and the real payload was undefined; subsequent
invalid-params errors from Core matched the /invalid/ classifier
and masqueraded as 422 core_rejected for otherwise valid
proposals. Fix: always pass the full canonical tuple we just
hashed. Regression test asserts on the exact arg shape.

P2 (lib/proposalDispatcher.js): /already exists/ was in the
TERMINAL_CORE_ERRORS list, so a dispatcher retry after a prior
successful gObject_submit (crash before markSubmitted, overlapping
workers, operator running two dispatchers, etc.) would flip the
row to failed and fire a false failure email, even though the
governance object is live on-chain. Fix: classify duplicate-submit
as a transient error, log it loudly via submit_already_exists, and
let the next tick retry so the existing governance_hash_clash path
in the status handler can promote it to submitted naturally.
Added a test that asserts the row stays awaiting_collateral and
no failure transition fires.

Full BE suite green (29 suites / 674 tests).

Made-with: Cursor
@sidhujag
Copy link
Copy Markdown
Member Author

@codex review

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ded00443dc

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread routes/govProposals.js Outdated
Comment on lines +553 to +556
const existing = submissions.findByProposalHashForUser(
userId,
proposalHash
);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Fix /prepare idempotency key to avoid time-based duplicates

The idempotency check in POST /prepare keys only on proposalHash, but proposalHash is computed from timeUnix generated from now() for each request in the same handler. That means two retries with identical proposal fields that cross a one-second boundary produce different hashes and bypass this lookup, creating duplicate prepared rows for the same logical submission. This contradicts the documented idempotency contract and can leave users with multiple pending submissions for one proposal.

Useful? React with 👍 / 👎.

Comment thread lib/proposalDispatcher.js Outdated
Comment on lines +249 to +253
} else if (isDuplicateCoreError(err)) {
// "Governance object already exists" — the object is live on
// chain, but we don't yet have its hash locally (likely a
// crash between gObject_submit succeeding and markSubmitted
// persisting, or a second dispatcher/operator submitted it).
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Resolve duplicate-submit rows instead of leaving them pending

In the duplicate-submit branch, the dispatcher explicitly leaves the row in awaiting_collateral and only logs. If gObjectSubmit keeps returning "already exists" for that object, this row has no transition path to either submitted or failed, so it can remain indefinitely in a non-terminal state with no completion signal to the user. This should reconcile to a terminal status (or recover the governance hash) rather than relying on unbounded retries.

Useful? React with 👍 / 👎.

…te-submit resolution)

Two new P1 findings from Codex round 2 on PR #8 — the first-round
fixes opened up secondary issues that only showed up under retry
scenarios. Both are real production hazards, not just reviewer
taste.

P1 #2 (routes/govProposals.js, lib/proposalSubmissions.js):
`proposalHash` bakes in `time` (seconds since epoch) — part of the
CGovernanceObject::GetHash() input tuple Core signs under. The
old idempotency check keyed /prepare on `proposalHash`, so two
retries of the same logical /prepare that straddled a one-second
boundary hashed differently and BOTH landed in the DB as
`prepared`, leaving the user with duplicate pending submissions
for one proposal. Fix: add findPreparedByDataHexForUser() on the
submissions repo (scoped to owner, status=prepared, ordered by
recency) and make /prepare look up by the time-free canonical
payload first. On hit we reuse the frozen {time, proposalHash}
from the existing row, skip the RPC pre-flight (Core already
accepted it and rate-limits repeats), and return 200 idempotent.
Regression test injects a monotonic now() and advances the
clock >1s between two identical POST /prepare calls, asserts
single row, same id, same hash, same time.

P1 #3 (lib/proposalDispatcher.js): the round-1 fix that stopped
flipping "already exists" to `failed` left the row in
`awaiting_collateral` forever if Core kept returning the
duplicate error — no terminal transition, no completion signal
to the user. Core's govobj store is keyed on
CGovernanceObject::GetHash(parent, rev, time, vchData, outpoint,
sig), which computeProposalHash() reproduces exactly at prepare
time (outpoint/sig are empty for top-level user proposals), so
the on-chain hash MUST equal row.proposalHash. Resolve by
calling markSubmitted({ governanceHash: row.proposalHash })
inside the duplicate branch. If the repo rejects with
governance_hash_clash (rare race against a parallel tick or a
coincidentally-identical submission), we log and leave the
state machine to reconcile — still no false-failure email, but
now we have a real terminal path. Two new tests cover both
outcomes.

Full BE suite green (29 suites / 676 tests).

Made-with: Cursor
@sidhujag
Copy link
Copy Markdown
Member Author

@codex review

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 548d50959f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +237 to +239
CREATE TABLE proposal_drafts (
id INTEGER PRIMARY KEY AUTOINCREMENT,
user_id INTEGER NOT NULL REFERENCES users(id) ON DELETE CASCADE,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Add a new migration for proposal tables

These schema changes were appended to 001_init.sql, but this codebase applies migrations only once per filename (schema_migrations), so any existing deployment that already recorded 001_init.sql will never run these CREATE TABLE statements. After upgrading, /gov/proposals will hit runtime SQL errors (no such table: proposal_drafts / proposal_submissions) on production databases. Ship these DDL changes in a new migration file (e.g., 002_...sql) so upgrades actually apply them.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

N/A

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged, but this is a false positive relative to this project's explicit pre-launch convention.

See the header of db/migrations/001_init.sql:

Until v1 ships, this file is the schema. Any rename, column addition, or constraint change that lands BEFORE launch belongs here, edited in place. Do NOT propose a forward ALTER TABLE migration as a "safer" alternative — there is no deployed database to migrate from, so a forward migration would only add file-count noise while describing a history that never existed. Once v1 is in production, this rule inverts and every schema change becomes a new 002+ file.

Nothing is deployed yet. The schema_migrations table is empty on every environment, so editing 001_init.sql in place does run the new CREATE TABLE / CREATE INDEX statements on first boot. The moment v1 ships and 001_init.sql is recorded in any production schema_migrations, this rule inverts and every subsequent schema change will land in a new 002+ file — that is exactly what the header calls out.

Keeping the review signal tight: marking this as "does not apply in this iteration". The new partial unique index (idx_proposal_submissions_user_payload_prepared added in 2e479f9) lives in 001_init.sql in keeping with that convention.

Comment thread routes/govProposals.js Outdated
Comment on lines +484 to +487
const existingByPayload = submissions.findPreparedByDataHexForUser(
userId,
canon.dataHex
);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Enforce prepare idempotency atomically

The payload-idempotency check is performed before the transactional insert, and there is no unique DB constraint for (user_id, data_hex, status='prepared'). Two concurrent /prepare requests with the same payload can both miss existingByPayload and both insert, violating the route’s idempotency contract and creating duplicate prepared submissions for the same proposal text.

Useful? React with 👍 / 👎.

Comment thread lib/proposalSubmissions.js Outdated
Comment on lines +309 to +313
db.prepare(
`UPDATE proposal_submissions
SET status = ?, collateral_txid = ?, updated_at = ?
WHERE id = ? AND user_id = ?`
).run(STATUS.AWAITING_COLLATERAL, txidLower, now(), id, userId);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Translate collateral txid unique races into 409

attachCollateral does a read-before-write duplicate check, but the uniqueness is ultimately enforced by a partial unique index at update time. Under concurrent requests attaching the same txid, one update can throw a raw SQLite constraint error here; because the route only maps custom .code values, that path returns 500 instead of the intended 409 txid_already_used. Catch and normalize constraint errors in this write path.

Useful? React with 👍 / 👎.

Address the two P2 findings from Codex's latest pass on PR #8 by
turning the read-before-write idempotency/duplicate checks into
DB-enforced invariants and normalising the remaining SQLite
constraint errors into stable application-level codes.

1. /prepare — payload-keyed idempotency is now atomic (Codex P2)
   - New partial unique index on proposal_submissions:
     (user_id, data_hex) WHERE status = 'prepared'. Two concurrent
     /prepare calls with identical canonical payloads can no longer
     both insert; the second write is rejected at the SQLite layer.
   - Route catches SQLITE_CONSTRAINT_UNIQUE from the create() path
     and rehashes/returns the winner's row as a 200 idempotent
     response, matching the pre-read idempotent branch. Caller can't
     tell whether the pre-read or the constraint fallback served it,
     which is the whole point.

2. attachCollateral — write-level race normalised (Codex P2)
   - Repo wraps the status='awaiting_collateral' UPDATE in a
     try/catch; SQLite's partial unique index on collateral_txid
     translates to the same `txid_already_used` code that the
     pre-read clash throws, so the route layer keeps returning 409
     regardless of which branch fires.

3. Pre-launch migration convention is preserved (Codex P1)
   - Codex flagged the new DDL as "needs a 002_ migration". The
     header in db/migrations/001_init.sql explicitly opts out of
     that rule pre-launch — 001 is the single source of truth until
     v1 ships. The new partial index lives in 001_init.sql in
     keeping with that convention. (Will flip to 002+ files for
     every schema change post-launch.)

Tests
- New: repo test that two prepared rows with same (user_id, data_hex)
  are rejected at the DB layer; different users may each hold a
  prepared row with the same dataHex; the partial predicate releases
  once the first row leaves 'prepared'.
- New: repo test that simulates the attach-collateral race by
  planting a competing row between the pre-read and the UPDATE; the
  raw SQLite constraint error is translated to txid_already_used.
- New: route test that forces the pre-read miss once and verifies
  the /prepare constraint fallback returns 200 idempotent with the
  winner's submission and the DB still holds exactly one row.
- Adjusted existing tests that happened to rely on being able to
  create two prepared rows with the same dataHex (listByStatus,
  proposalDispatcher multi-row test, db.test.js collateral_txid
  uniqueness fixture) to use distinct data_hex values — the intent
  of each test is unchanged.

All 682 backend tests green.

Made-with: Cursor
@sidhujag
Copy link
Copy Markdown
Member Author

@codex review

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2e479f9547

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread db/migrations/001_init.sql
Comment thread lib/proposalSubmissions.js Outdated
Comment on lines +313 to +316
`UPDATE proposal_submissions
SET status = ?, collateral_txid = ?, updated_at = ?
WHERE id = ? AND user_id = ?`
).run(STATUS.AWAITING_COLLATERAL, txidLower, now(), id, userId);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Include status guard in attach-collateral UPDATE

attachCollateral() checks that the row is prepared before writing, but the actual UPDATE only filters by id/user_id. In a multi-worker/process deployment, two racing attach requests can both pass the pre-read and the later write can overwrite collateral_txid on a row already moved to awaiting_collateral, violating the state machine and potentially binding the submission to the wrong collateral transaction. The write should enforce the expected old status in SQL (or equivalent compare-and-swap semantics).

Useful? React with 👍 / 👎.

jagdeep sidhu added 2 commits April 21, 2026 17:05
Adds a minimal GitHub Actions workflow that installs with `npm ci`
and runs `npm test -- --runInBand --ci` on Node 20 and Node 22
(both in LTS). --runInBand keeps the SQLite test suites
deterministic on the hosted runner and avoids the rare cross-test
`:memory:` db leakage we saw locally on multi-worker jobs.

Also adds a cheap schema-sanity job that fails if two migration
filenames share the same numeric prefix — the runner in `lib/db.js`
sorts lexicographically, so colliding prefixes make apply order
undefined. Exactly the kind of silent footgun CI should catch.

concurrency: ci-<workflow>-<ref> with cancel-in-progress so a
force-push supersedes the previous run rather than queueing.

Made-with: Cursor
…dex R4 P1)

Context: the prior attachCollateral() pre-read the row, verified
status='prepared', then issued an UPDATE that filtered only on
id/user_id. In a multi-worker deployment, two racing attach
requests could both pass the pre-read and both UPDATE — the second
would silently overwrite the first's collateral_txid, binding the
submission to the wrong collateral transaction. The partial unique
index on collateral_txid only catches the case where both racers
are attaching the SAME txid; divergent txids would pass.

Fix: fold the expected status into the UPDATE's WHERE clause
(`AND status = 'prepared'`) so the write itself implements a
compare-and-swap. On zero-changes, re-read the row and:
  - return null if it disappeared (remove())
  - throw `status_not_prepared` with the row's real current state
    if a racer already advanced it
  - throw `attach_cas_miss` (retryable) as a defensive fallback

Existing `txid_already_used` SQLITE_CONSTRAINT normalization is
preserved around the new CAS UPDATE.

Test: adds a regression that plants a concurrent status-flip
between the pre-read and UPDATE via a narrowly-scoped db.prepare
monkey-patch, then verifies:
  1. the attempt throws `status_not_prepared`, and
  2. the row's collateral_txid remains the winner's, never the
     loser's.

All 683 backend tests pass on Node 20.19.4.

Made-with: Cursor
@sidhujag
Copy link
Copy Markdown
Member Author

@codex review. Don't worry about migrations, not an issue.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 40dcb4eb58

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread routes/govProposals.js Outdated
// SYS. Transient errors (node unreachable) are soft-treated —
// we proceed and trust the dispatcher to surface any issue later
// when it calls gObject_submit.
if (typeof rpc.gObjectCheck === 'function') {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Guard optional RPC before dereferencing gObjectCheck

The route advertises rpc as optional, but this dereference assumes it is always an object. In the same commit, mountAuthAndVault passes proposalRpc through with a default of null, so a default createApp() deployment can hit /gov/proposals/prepare and throw TypeError: Cannot read properties of null (reading 'gObjectCheck') instead of gracefully skipping preflight. Because this happens inside an async handler without an outer catch, it can surface as an unhandled route failure rather than a structured API error.

Useful? React with 👍 / 👎.

Comment thread routes/govProposals.js
let timeUnix;
let proposalHash;
let opReturnHex;
if (existingByPayload) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Run Core preflight before idempotent prepare short-circuit

This early-return branch bypasses the gObject_check preflight entirely for retries. If the first /prepare call created the row during a transient RPC outage (the code explicitly soft-allows network errors), then a later retry with the same payload will always return this cached prepared row and never revalidate against Core, even after RPC recovers. That can let Core-only-invalid proposals (for example, checksum-invalid payment addresses) proceed to collateral payment and fail only in the dispatcher after the 150 SYS fee is already burned.

Useful? React with 👍 / 👎.

…odex R5 P1)

Two related prepare-route fixes:

1. `rpc` is advertised as optional and `appFactory.js` explicitly passes
   `null` when no Core connection is wired. Destructured defaults only
   fire for `undefined`, so an explicit null flowed through and the bare
   `typeof rpc.gObjectCheck` dereference threw TypeError inside an async
   handler, surfacing as an unhandled rejection instead of a clean
   "skip preflight". Guard with `rpc &&` before dereferencing.

2. The idempotent-replay branch short-circuited before re-running the
   Core preflight. If the original /prepare landed during a transient
   RPC outage (the code soft-allows network errors), every retry would
   replay the cached row and never revalidate once Core recovered. A
   Core-invalid proposal could then proceed to burn 150 SYS before
   failing in the dispatcher. Moved preflight BEFORE the idempotency
   short-circuit so every /prepare response is backed by a fresh Core
   ack (or an explicit soft-fail we logged). Added regression test.

Made-with: Cursor
@sidhujag
Copy link
Copy Markdown
Member Author

@codex review. Don't worry about migrations, not an issue.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fcd965e995

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread lib/proposalSubmissions.js Outdated
Comment on lines +421 to +424
`UPDATE proposal_submissions
SET status = ?, governance_hash = ?, updated_at = ?
WHERE id = ?`
).run(STATUS.SUBMITTED, gh, now(), id);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Guard markSubmitted with state compare-and-swap

markSubmitted does a read-time status check, but the UPDATE only filters by id, so two dispatcher workers processing the same row can both read awaiting_collateral and both commit a "successful" transition. In that race, each worker can treat the transition as its own success and fire duplicate submitted-side effects (notably duplicate emails), despite the dispatcher expecting a raced second writer to get status_not_awaiting. Add AND status = 'awaiting_collateral' (CAS semantics) and treat zero changed rows as a raced no-op.

Useful? React with 👍 / 👎.

Comment thread routes/govProposals.js
Comment on lines +134 to +137
if (f.paymentAmountSats !== undefined) {
// Digit-string or number; proposalDrafts will bigint-coerce.
patch.payment_amount_sats = f.paymentAmountSats;
} else if (f.paymentAmount !== undefined) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Validate paymentAmountSats before writing draft rows

When paymentAmountSats is provided, this path forwards it unvalidated to the repo; malformed values (for example "12.5" or non-digit strings) then throw inside proposalDrafts.create/update, which the route catches as a generic error and returns 500 internal instead of a user-facing 400 validation_failed. Since this is request-shape validation, it should be rejected here consistently like the paymentAmount branch.

Useful? React with 👍 / 👎.

…untSats validation (Codex R5 P1/P2)

Backend round 5 follow-ups from the Codex review loop on PR8:

* markSubmitted now does a compare-and-swap on status
  (`WHERE id = ? AND status = 'awaiting_collateral'`). Losing workers
  get back `null` instead of throwing, so two dispatchers that race on
  the same row no longer both emit `onSubmitted` side effects.
* proposalDispatcher handles both paths of the CAS:
  - null return (CAS miss): log `markSubmitted_raced` with
    `code: 'cas_miss'`
  - exception (`status_not_awaiting` / `governance_hash_clash`): log
    once and set a guard so the CAS-miss branch does not re-log.
* /gov/proposals/drafts POST + PATCH validate `paymentAmountSats`
  before forwarding to the repo. Previously malformed values
  (`"12.5"`, `"abc"`, `""`, `"-1"`, `"007"`, `"1e3"`, ...) bubbled out
  of `toBigIntSats()` as a generic 500. Route layer now returns
  the canonical `400 { error: 'validation_failed', issues: [...] }`
  shape with `code: 'amount_sats_invalid'`.

Regression tests added:
* proposalSubmissions.test.js — markSubmitted CAS miss returns null
  without throwing and preserves the winning row's state.
* govProposals.routes.test.js — 400 for malformed paymentAmountSats
  on both POST and PATCH; happy-path digit string still accepted.

All 688 backend tests pass.

Made-with: Cursor
@sidhujag
Copy link
Copy Markdown
Member Author

@codex review. Don't worry about migrations, not an issue.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 960418c282

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread server.js Outdated
Comment on lines +173 to +175
return rpcServices(client.callRpc)
.gObject_check(parentHash, String(revision), String(time), dataHex)
.call();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Call gObject_check with its single hex_data argument

Syscoin Core's gobject_check RPC accepts only one parameter (hex_data), but this adapter sends four (parentHash, revision, time, dataHex). On a real node that returns RPC_INVALID_PARAMETER, and /gov/proposals/prepare classifies that as a terminal validation error (/invalid/), so otherwise-valid proposal prepares are rejected with 422 whenever proposalRpc is enabled.

Useful? React with 👍 / 👎.

Comment thread routes/govProposals.js Outdated
Comment on lines +576 to +582
const okFlag = result && (result.Object || result.object);
// Core 4.x returns { "Object": "success" } on accept. Anything
// else is treated as a rejection; parse the message for codes.
if (
okFlag &&
String(okFlag).toLowerCase() === 'success'
) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Treat Core's successful gobject_check shape as accepted

The preflight success check only accepts result.Object === 'success', but Syscoin Core returns success as { "Object status": "OK" } for gobject_check. That means successful Core validation is interpreted as rejection and /prepare returns 422 core_rejected even for valid payloads; this still breaks proposal creation even after fixing the RPC argument count.

Useful? React with 👍 / 👎.

…s": "OK" } (Codex R6 P1)

Two tightly-coupled Codex findings against the proposal preflight,
both surfaced as "otherwise-valid proposals get 422 core_rejected on
a real Core node":

1. Adapter signature: Syscoin Core's `gobject_check` RPC takes
   exactly ONE positional arg, `hex_data` (see
   syscoin/src/rpc/governance.cpp::gobject_check — it constructs
   the govobj with uint256() / 1 / GetAdjustedTime() internally and
   derives nothing from the caller). Our adapter was modelled on
   `gobject_submit` and forwarded (parentHash, revision, time,
   dataHex); Core rejects that with RPC_INVALID_PARAMS ("too many
   positional arguments") and the route's terminal-error classifier
   then flagged that as 422 core_rejected. Adapter + route now pass
   just `canon.dataHex`.

2. Success response shape: Core returns
     { "Object status": "OK" }
   (literal key with a space; governance.cpp line 111). Our check
   looked for `result.Object === 'success'`, which never matches a
   real Core response, so every successful preflight fell through
   to the reject branch. Route now accepts the real shape, with a
   case-insensitive compare on the OK string for forward-compat
   but a strict key match.

Regression tests:
* happy-path preflight mock updated to produce the real shape and
  asserts the adapter sees exactly one arg (the hex_data).
* new: "accepts Core's canonical success shape" — sends the exact
  "Object status: OK" response and expects 201.
* new: "gObjectCheck is called with hex_data only" — pins the
  adapter signature at the route boundary so a future regression
  would fail fast.
* existing "core_rejected when gObjectCheck returns non-success"
  updated to a rejection shape that no longer pretends to set
  `Object: 'failure'` (Core never emits that key).

All 690 backend tests pass.

Made-with: Cursor
@sidhujag
Copy link
Copy Markdown
Member Author

@codex review. Don't worry about migrations, not an issue.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 481d7586c0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread lib/proposalSubmissions.js Outdated
Comment on lines +460 to +463
db.prepare(
`UPDATE proposal_submissions
SET status = ?, fail_reason = ?, fail_detail = ?, updated_at = ?
WHERE id = ?`
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Guard markFailed with status compare-and-swap

markFailed does a status pre-read but then updates by id only, so a concurrent worker can move the same row to submitted between those two operations and this write will still overwrite it back to failed. In a multi-worker dispatcher setup, that corrupts the terminal state and can trigger the wrong side effects (failure path after successful submission). This transition needs the same CAS-style status guard used in markSubmitted (or a checked changes===0 path) to avoid clobbering raced updates.

Useful? React with 👍 / 👎.

Comment thread routes/govProposals.js Outdated
Comment on lines +881 to +882
submissions.remove(id, userId);
return res.status(204).end();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Verify submission deletion actually removed a row

This handler always returns 204 after calling submissions.remove, but remove is conditional on status (prepared/failed) and can legitimately return 0 if the row changes state after the pre-read (for example, a concurrent attach-collateral request flips it to awaiting_collateral). That creates a false-success delete response while the submission still exists. The result of remove should be checked and mapped to a conflict/not-found response.

Useful? React with 👍 / 👎.

… a row (Codex R7 P1/P2)

Two race-safety follow-ups against the proposal submission lifecycle:

* `markFailed` had a read-time status check but the UPDATE only filtered by
  id, so two dispatcher workers processing the same row could have one
  succeed in `markSubmitted` while the other's "terminal reject" path
  (e.g. gObjectSubmit throws a validation-ish error) stomps it back to
  `failed` — corrupting the terminal state and firing onFailed after an
  onSubmitted already went out. Now UPDATE gates on
  `AND status NOT IN ('submitted', 'failed')` and returns null on a CAS
  miss. Dispatcher's `if (failedRow)` guard already skips the hook on
  null, so the winner keeps emit-once semantics.

* `DELETE /submissions/:id` pre-reads the row, confirms it is
  `prepared` or `failed`, then calls `submissions.remove`. That remove
  uses a partial DELETE guarded on the same status set, so a concurrent
  transition (another tab attach-collateral, dispatcher pickup) between
  pre-read and DELETE produces `changes = 0`. The handler used to
  return 204 regardless — a false-success that tells the client the
  submission is gone while it is in fact still live and may run to
  completion on-chain. Route now checks the row count, re-reads on a
  miss, and returns 404 (row deleted by another tab) or 409
  status_not_deletable (raced to a non-deletable state).

Regression tests:
* proposalSubmissions.test.js — CAS-miss on markFailed returns null
  without throwing; winning `submitted` row is preserved.
* govProposals.routes.test.js — DELETE on a row raced to
  `awaiting_collateral` returns 409 and the row is still visible.
* govProposals.routes.test.js — DELETE that finds 0 changes because
  the row was consumed elsewhere returns 404.

All 693 backend tests pass.

Made-with: Cursor
@sidhujag
Copy link
Copy Markdown
Member Author

@codex review. Don't worry about migrations, not an issue.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6e5a0d9b66

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread routes/govProposals.js Outdated
Comment on lines +520 to +523
const existingByPayload = submissions.findPreparedByDataHexForUser(
userId,
canon.dataHex
);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Catch repo errors in async prepare route

This submissions.findPreparedByDataHexForUser(...) call sits outside any top-level try/catch in an async Express 4 handler, so a synchronous DB exception (for example SQLITE_BUSY/I/O errors) becomes an unhandled promise rejection instead of a controlled JSON 500 response. In production that can surface as hung requests or process-level instability under transient database faults, so this lookup should be wrapped by the same error handling used for other persistence calls in this route.

Useful? React with 👍 / 👎.

Comment thread lib/appFactory.js
Comment on lines +344 to +347
dispatcherTimer = setTimeout(
fireAndSchedule,
proposalDispatcherIntervalMs
);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Stop proposal dispatcher from rearming after stop

stopProposalDispatcher() clears the current timeout, but fireAndSchedule() always creates a new timeout after await dispatcher.tick(). If stopProposalDispatcher() is called while a tick is in flight, the callback still executes this block and re-arms the loop, so the dispatcher keeps running after it was asked to stop (notably in teardown/shutdown paths).

Useful? React with 👍 / 👎.

Codex PR8 round 8 fixes.

P1 (routes/govProposals.js): the async /prepare handler had multiple
persistence calls (findPreparedByDataHexForUser,
drafts.getByIdForUser) outside any try/catch. A synchronous
better-sqlite3 throw (SQLITE_BUSY, I/O, corrupt-index) became an
unhandled promise rejection rather than a controlled JSON 500 —
Express 4 does not catch async handler rejections. Wrap both calls
so transient DB faults surface as clean 500s the client can retry.
Regression test forces SQLITE_BUSY on the lookup and asserts a 500
with no unhandled rejection leak.

P2 (lib/appFactory.js): stopProposalDispatcher() only called
clearTimeout on a pending timer. If a tick was already in flight
(past `await dispatcher.tick()`), the callback still re-armed the
loop after resolving, so stop was effectively ignored — leaking the
dispatcher into teardown and shutdown paths. Add a
`dispatcherStopped` flag, check it before AND after the in-flight
tick, and set it in stopProposalDispatcher. Regression test stubs
the dispatcher with a controllable deferred, stops while a tick is
parked on it, then releases it; asserts no further ticks fire.

Made-with: Cursor
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 591956117d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread lib/proposalDispatcher.js
Comment on lines +280 to +284
log('warn', 'markSubmitted_after_duplicate_failed', {
id,
code: markErr && markErr.code,
msg: String((markErr && markErr.message) || markErr),
});
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Transition duplicate-hash clashes out of awaiting_collateral

When gObject_submit returns an "already exists" error, this path tries markSubmitted, but if that throws governance_hash_clash (because another row already claimed the governance hash) the handler only logs and exits. The submission remains in awaiting_collateral, so every dispatcher tick retries forever, repeatedly hitting the same duplicate condition and never producing a terminal user-visible outcome even though collateral is already paid.

Useful? React with 👍 / 👎.

Comment thread lib/proposalDispatcher.js Outdated
// (Codex PR8 round 1 P2.)
const TERMINAL_CORE_ERRORS = [
/rate limit/i,
/invalid/i,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Tighten terminal error matching to avoid false failures

Including /invalid/i in terminal error patterns is too broad: transient RPC transport/parser failures frequently contain the word "invalid" (for example, invalid JSON-RPC response text), so recoverable outages can be misclassified as permanent submit rejections. In those cases rows are marked failed instead of retried, which is a user-impacting incorrect terminal transition.

Useful? React with 👍 / 👎.

…ns (Codex R10 P1/P2)

Two backend fixes from Codex PR8 round 10 review:

P1 (lib/proposalDispatcher.js) — duplicate governance-hash clashes
must transition out of awaiting_collateral. When Core returns
"already exists" and markSubmitted then throws
governance_hash_clash (another row already claimed this
governance_hash via the UNIQUE index), round 9 code only logged
and left the row in awaiting_collateral — so the dispatcher
retried that same row every tick forever, producing no terminal
user-visible outcome even though the on-chain object exists and
the collateral is already spent. Now flip the clashing row to
terminal `failed` with reason `duplicate_governance_hash` via the
same CAS path used elsewhere, and fire onFailed so the user gets
a clean notification. A follow-up tick confirms the terminal row
is no longer picked up.

P2 (lib/proposalDispatcher.js) — tighten TERMINAL_CORE_ERRORS.
The previous list had a blanket /invalid/i pattern that was far
too broad: transient RPC transport / JSON-parser errors ("invalid
JSON-RPC response", "invalid response from server", "invalid
utf-8 sequence", etc.) frequently contain the word "invalid",
and classifying those as permanent rejections flipped rows to
`failed` on temporary outages and fired user-visible failure
emails the user could never fix. Narrow to the exact phrases
Syscoin Core actually emits for governance-object validation
failures (`Governance object is not valid`, `Invalid parent
hash`, `Invalid (object )?signature`, `Invalid object type`,
`Invalid proposal`, `Invalid data hex`, `hash mismatch`) plus the
existing `Object submission rejected` and `rate limit`. Anything
else — bare "invalid" in a transport string — stays classified
as transient and is retried next tick.

Updated an existing test that asserted the hash-clash row stayed
in awaiting_collateral; added a new regression test asserting
the terminal + onFailed + no-retry behavior. Added a separate
regression test for the invalid-transport vs. Core-terminal
distinction. Full BE suite 697/697 passing.

Made-with: Cursor
@sidhujag
Copy link
Copy Markdown
Member Author

@codex review. Don't worry about migrations, not an issue.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 601eeedd48

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread routes/govProposals.js Outdated
Comment on lines +623 to +624
/validation|invalid|exceeds|rejected|collateral|size/i.test(msg)
) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Narrow gObjectCheck terminal-error matcher

POST /gov/proposals/prepare is designed to soft-allow transient RPC outages, but this branch classifies any exception containing invalid as a permanent Core rejection and returns 422 core_rejected. JSON-RPC transport failures commonly include text like “invalid response”/“Invalid URL”, so temporary node or network issues can be misreported as proposal-validation failures and block users from preparing submissions until the outage clears. Restrict this matcher to actual governance-validation phrases (or explicitly exclude transport errors) before returning 422.

Useful? React with 👍 / 👎.

Comment thread lib/proposalDispatcher.js
Comment on lines +318 to +319
clashed = markErr && markErr.code === 'governance_hash_clash';
log('warn', 'markSubmitted_after_duplicate_failed', {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Handle unique-hash races in duplicate-submit recovery

In the duplicate-submit recovery path, only markSubmitted errors with code === 'governance_hash_clash' are treated as terminal duplicates. Under concurrent workers, the same scenario can surface as raw SQLITE_CONSTRAINT(_UNIQUE) from the unique governance_hash index (race between pre-check and UPDATE), which leaves clashed false and the row stuck in awaiting_collateral to be retried indefinitely. This branch should also treat unique-constraint errors as a clash (or ensure the repo normalizes them) so the row can transition to the terminal duplicate-failure state.

Useful? React with 👍 / 👎.

…ash races

P1 (routes/govProposals.js): narrow gObjectCheck terminal-error
matcher. Previous heuristic included a bare /invalid/ token, which
matched JSON-RPC transport/parser errors that routinely contain the
word "invalid" ("Invalid URL", "invalid response from server",
"invalid JSON-RPC response", "invalid utf-8 sequence in headers").
Those are transient outages, not Core validation rejects, but the
old matcher misreported them as permanent 422 core_rejected and
blocked legitimate /prepare calls until the node/network recovered.

Replaced with an explicit allowlist of phrases Syscoin Core actually
emits from CGovernanceObject::IsValidLocally() and gobject_check's
reject branches (name exceeds, data exceeds, url invalid,
payment_address invalid, start_epoch, type field not found, ...)
plus the gobject-submit wrapper rejects (Governance object is not
valid, Object submission rejected, Invalid parent hash, signature/
object-type/data-hex rejects, collateral rejected). Everything
else falls through to soft-allow. Deliberately does NOT use
parseCoreRejectMessage for classification because its catch-all
arm raises a generic core_rejected issue for ANY non-empty string,
which would false-positive every transport error. Added 11
regression tests: 5 transport-error strings that must NOT terminate
and 6 genuine Core reject phrases that still must 422.

P2 (lib/proposalSubmissions.js): normalize raw SQLITE_CONSTRAINT_
UNIQUE on governance_hash to governance_hash_clash. Under
concurrent dispatcher workers, the read-above (byGovHashStmt
pre-check) + write-here (UPDATE) pair is not atomic — two workers
can each see clash=null, then the second UPDATE hits the partial
UNIQUE index on `governance_hash` and rejects with SQLITE_CONSTRAINT_
UNIQUE. Pre-fix the raw error bubbled up unwrapped; the dispatcher's
duplicate-recovery branch only treats code === 'governance_hash_
clash' as a terminal clash, so the raw error left clashed=false,
the row stayed awaiting_collateral, and every subsequent tick
re-hit Core's "already exists" and looped forever with no
terminal user-visible outcome. Wrap the UPDATE in try/catch and
rethrow as governance_hash_clash (same normalization pattern as
attachCollateral's collateral-txid unique clash). Regression test
exercises the code path by patching db.prepare to force the
pre-check to miss so the UPDATE is the boundary that raises.

Made-with: Cursor
@sidhujag
Copy link
Copy Markdown
Member Author

@codex review. Don't worry about migrations, not an issue.

1 similar comment
@sidhujag
Copy link
Copy Markdown
Member Author

@codex review. Don't worry about migrations, not an issue.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 56c4491be5

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread lib/proposalDispatcher.js Outdated
// transport string — stays classified as transient and gets
// retried.
const TERMINAL_CORE_ERRORS = [
/rate limit/i,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Narrow terminal rate-limit matching in submit errors

Treating any message that contains rate limit as terminal causes false permanent failures when the RPC path returns transient throttling errors (for example temporary 429s from a proxy/provider). In that case isTerminalCoreError returns true, the row is marked failed with submit_rejected, and retries stop even though the submission may succeed later. Match only the explicit Syscoin Core governance rejection phrase instead of a broad token.

Useful? React with 👍 / 👎.

Comment thread routes/govProposals.js Outdated
Comment on lines +827 to +830
const winner = submissions.findPreparedByDataHexForUser(
userId,
canon.dataHex
);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Guard winner re-read in prepare race fallback

In the unique-constraint recovery path, submissions.findPreparedByDataHexForUser(...) is called without a local try/catch inside an async Express 4 handler. If that DB read throws (e.g., transient SQLite error), the rejection escapes and can become an unhandled async route failure instead of returning the expected JSON 500. This reintroduces the async error-handling gap this route is otherwise trying to avoid.

Useful? React with 👍 / 👎.

…-read

P1 (lib/proposalDispatcher.js): replace bare /rate limit/i in
TERMINAL_CORE_ERRORS with the exact Syscoin Core phrase
"Object creation rate limit exceeded" (thrown from gobject(submit)
at syscoin/src/rpc/governance.cpp:204). The old pattern matched
transient proxy/provider 429 responses whose bodies routinely
contain "rate limit"/"rate-limited" — those should be retried,
not terminate the row. Core's phrase is the one permanent-reject
condition where the object hash is burned for this cycle.

Regression test: a proxy 429 "rate limit exceeded" body leaves
the row in awaiting_collateral, while Core's exact phrase flips
to failed/submit_rejected on the next tick.

P2 (routes/govProposals.js): wrap the winner re-read in the
unique-constraint recovery path (submissions.findPreparedByData
HexForUser) in try/catch. better-sqlite3 is synchronous and can
throw (SQLITE_BUSY, I/O, corrupt index) from that read; without
the guard the throw escaped into the async Express 4 handler as
an unhandled rejection, which in prod surfaces as a process-
level UnhandledPromiseRejection warning and a client-visible
500 HTML default page instead of the handler's structured
{ error: 'internal' } JSON. Returns 500 JSON on lookup failure,
matching the pattern used for the other DB reads in this route.

Regression test: prime a prepared row so the UNIQUE index will
reject a second insert; make the pre-read miss (drives the
recovery branch) AND throw SQLITE_BUSY on the re-read; assert
the second /prepare returns { error: 'internal' } JSON 500.

Made-with: Cursor
@sidhujag
Copy link
Copy Markdown
Member Author

@codex review. Don't worry about migrations, not an issue.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 386108c5e2

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread lib/proposalDispatcher.js Outdated
Comment on lines +423 to +426
// status_not_awaiting: someone else moved the row ahead of us.
// governance_hash_clash: we computed the same hash again for
// the same row (safe; the UNIQUE index prevents a second row).
log('warn', 'markSubmitted_raced', {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Mark clashed submissions failed after successful gObjectSubmit

In the success path, a markSubmitted throw with governance_hash_clash is currently treated as a harmless race and only logged, but that error means a different row already owns the same governance hash. When Core returns a hash for a duplicate submit (instead of an "already exists" error), this leaves the current row stuck in awaiting_collateral and retried forever with no terminal user-visible outcome. This branch should mirror the duplicate-error handling and transition the row to terminal failed (e.g., duplicate_governance_hash).

Useful? React with 👍 / 👎.

@sidhujag
Copy link
Copy Markdown
Member Author

@codex review. Don't worry about migrations, not an issue.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 386108c5e2

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread lib/proposalValidate.js Outdated
Comment on lines +116 to +119
if (typeof input === 'number') {
if (!Number.isFinite(input)) throw new Error('payment_amount must be finite');
// Convert via string to avoid float drift: 42.5 -> "42.5" -> sats
return parsePaymentAmountToSats(input.toString());
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Parse numeric payment_amount without exponent rejection

parsePaymentAmountToSats documents support for numeric payment_amount, but this branch converts numbers with input.toString() and then validates with a decimal-only regex. For valid values like 0.00000001 (1 satoshi), JavaScript stringifies to 1e-8, which fails the regex and gets rejected as invalid. That makes /gov/proposals/prepare (and draft normalization paths that rely on this parser) return validation_failed for valid numeric inputs that common JSON clients can emit in scientific notation.

Useful? React with 👍 / 👎.

Backend:
- BE P1 (lib/proposalDispatcher.js): mark rows failed with
  duplicate_governance_hash when gObjectSubmit succeeds but the
  repo's UNIQUE index on governance_hash rejects the UPDATE.
  Previously this path only logged and returned, leaving the row
  stuck in awaiting_collateral and retried forever with no
  terminal outcome. Mirrors the existing duplicate-error branch
  and fires onFailed so the user gets a terminal notification.
- BE P2 (lib/proposalValidate.js): parsePaymentAmountToSats now
  handles numeric inputs that JavaScript stringifies to
  scientific notation (e.g. 0.00000001 -> "1e-8"). Route through
  toFixed(8) so the decimal regex accepts them; previously valid
  sats-scale numeric values surfaced as validation_failed.

Frontend:
- FE P2 (src/pages/NewProposal.js): always clear loadError when
  the ?draft= query is removed from the URL. Prior code gated
  the reset on draftId != null, so a failed load (which clears
  draftId in catch) left a stale "Couldn't load draft" banner
  visible on a fresh-new-proposal route.
- FE P2 (src/lib/proposalForm.js): draftBodyFromForm now accepts
  { forUpdate } and, when true, emits explicit empty strings for
  cleared text fields (name/url/paymentAddress) and null for
  cleared epochs. NewProposal passes forUpdate: true to
  updateDraft so user clears actually PATCH through. Previously
  dropping empty keys silently discarded a user's explicit
  delete of e.g. url.
- FE P2 (src/pages/ProposalStatus.js): extend the [id] reset
  effect to also clear deleteError, attachError, and txidInput.
  These are per-submission UI state; leaving them set across
  /governance/proposal/:id navigations showed stale error
  banners on a different row and — worse — prefilled row B's
  attach-collateral textbox with a txid the user pasted into
  row A.
- FE P3 (src/pages/ProposalStatus.js): treat not_found as
  non-retryable in the polling short-circuit. Previously the
  guard excluded only invalid_id and forbidden; a stale/deleted
  submission would otherwise keep re-firing getSubmission every
  60s indefinitely for every mounted tab.

Full regression tests added for each fix; existing test suites
continue to pass (BE 714 / FE 652).

Made-with: Cursor
@sidhujag
Copy link
Copy Markdown
Member Author

@codex review. Don't worry about migrations, not an issue.

1 similar comment
@sidhujag
Copy link
Copy Markdown
Member Author

@codex review. Don't worry about migrations, not an issue.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d3050d2a15

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread lib/proposalValidate.js Outdated
// already far exceeds total supply). Route through a fixed
// decimal form so the regex/BigInt path below stays the sole
// format of record.
return parsePaymentAmountToSats(input.toFixed(8));
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Reject over-precision numeric payment amounts

parsePaymentAmountToSats currently converts numeric inputs with toFixed(8), which rounds instead of validating precision. That means a request like paymentAmount: 0.000000009 is silently accepted as 1 sat (and 1.999999999 becomes 2 SYS) rather than being rejected for having more than 8 decimals, so the persisted/on-chain payment amount can differ from what the client sent. The string path correctly enforces <= 8 decimals, so the number path should preserve that behavior instead of mutating values.

Useful? React with 👍 / 👎.

Codex PR8 round 14 P1. `parsePaymentAmountToSats` previously routed
numeric inputs through `toFixed(8)` to sidestep scientific-notation
stringification. `toFixed` ROUNDS, so `0.000000009` silently became
1 sat and `1.999999999` became 2 SYS — the persisted/on-chain amount
diverged from what the client sent.

Replace the round-through-toFixed step with a non-mutating decimal
converter (`numberToDecimalString`) that preserves the number's exact
significant digits from its shortest round-trip form. The existing
string path's `<= 8 decimals` check then becomes the sole precision
gate: numeric `0.00000001` is still accepted as 1 sat, but numeric
`0.000000009`, `0.0000000099`, and `1.0000000001` are now rejected
with the same `payment_amount has more than 8 decimal places` error
that the string path emits.

Regression tests cover the rejection path and confirm that exact
8-decimal numeric values (1 sat, 1.00000001 SYS) still parse
losslessly.

Made-with: Cursor
@sidhujag
Copy link
Copy Markdown
Member Author

@codex review. Don't worry about migrations, not an issue.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 26be11d083

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread lib/proposalValidate.js
Comment on lines +365 to +367
typeof payload.payment_amount_sats !== 'bigint' ||
payload.payment_amount_sats <= 0n
) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Enforce int64 cap on proposal payment amounts

validateStructural only checks that payment_amount_sats is positive, so inputs above SQLite's signed 64-bit range (e.g. 9223372036854775808 sats) pass validation and reach persistence. In POST /gov/proposals/prepare, that overflows the payment_amount_sats INTEGER write in proposalSubmissions.create, which surfaces as a generic 500 instead of a deterministic 400 validation error; users can repeatedly hit an internal error with the same payload. Add an explicit upper-bound check before persistence (or in structural validation) to reject out-of-range amounts.

Useful? React with 👍 / 👎.

Codex PR8 round 15 P2. `validateStructural` only rejected
payment_amount_sats when it was non-positive, so values at or above
int64_max (e.g. 9_223_372_036_854_775_808 sats) slipped through and
hit the SQLite INTEGER write inside proposalSubmissions.create.
The column overflowed the signed 64-bit range and POST
/gov/proposals/prepare surfaced a generic 500 instead of a
deterministic 400 validation error — clients then had no structured
reason to correct the payload and would retry the same broken
request.

Introduce MAX_PAYMENT_AMOUNT_SATS = 2n**63n - 1n as an explicit
upper bound in structural validation, emitting a new
`amount_too_large` code so the frontend can map it to distinct copy
(separate from `amount_not_positive`). Syscoin's total supply cap
is ~9e15 sats, comfortably below this ceiling, so legitimate
proposals are unaffected.

Regression tests cover three cases:
- Exact int64_max (9.22e18 sats) accepted — no storage overflow.
- int64_max + 1 rejected with `amount_too_large`, not
  `amount_not_positive`.
- Wildly over-range amount (10^20 sats) still produces a clean
  validation failure rather than an engine-level overflow.

Made-with: Cursor
@sidhujag
Copy link
Copy Markdown
Member Author

@codex review. Don't worry about migrations, not an issue.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 748bf67b8d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread server.js Outdated
Comment on lines +165 to +166
.gObject_submit(parentHash, String(revision), String(time), dataHex, feeTxid)
.call(true);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Send numeric revision/time to gObject_submit

gobject_submit expects revision and time as numeric RPC params, but this adapter coerces both to strings before calling syscoind. In production this causes Core to reject submission calls with invalid-parameter errors once collateral reaches 6 confs; the dispatcher classifies those as transient and leaves rows stuck in awaiting_collateral, so proposals never transition to submitted or failed.

Useful? React with 👍 / 👎.

Comment thread routes/govProposals.js Outdated
Comment on lines +149 to +154
} else if (typeof sats === 'string') {
// Require digit-only with no leading/trailing whitespace and
// no leading zeros longer than 1 char (so "0" is fine but
// "007" is not — matches the canonical serialization we
// would later emit). An empty string is rejected.
isValid = /^(0|[1-9][0-9]*)$/.test(sats);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Enforce int64 ceiling for draft payment_amount_sats

Draft validation accepts arbitrarily large digit strings for payment_amount_sats but never bounds them to SQLite INTEGER range. A request with a very large value passes this check and then reaches proposal_drafts.payment_amount_sats insert/update, where the oversized BigInt can throw at the DB layer and is surfaced as 500 internal instead of a deterministic 400 validation_failed.

Useful? React with 👍 / 👎.

…ap on draft amounts

Codex PR8 round 16.

P1 (lib/proposalRpc.js + server.js): the production RPC adapter was
wrapping `revision` and `time` in `String(...)` before invoking
@syscoin/syscoin-js. syscoin-js forwards JS values to the daemon
as-is, so both params arrived as JSON strings; Syscoin Core's
`gobject_submit` declares them as `RPCArg::Type::NUM` and
`RPCTypeCheck` rejects the call with an
"Expected type number, got string" RPC_TYPE_ERROR. The dispatcher's
terminal/transient classifier treated that as transient and rows
stayed pinned to `awaiting_collateral` forever in production — no
proposal ever transitioned to `submitted` or `failed`. The String()
wrappers were a leftover from mirroring the syscoin-CLI shape
(rpc/client.cpp has `{"gobject_submit", 1, "revision"}` conversion
entries, but those rules are applied by the CLI before forwarding
and do not apply to direct JSON-RPC callers).

Extracted the adapter into `lib/proposalRpc.js` (factory pattern)
so the exact argument shapes sent to syscoin-js are now unit-
testable. Regression test asserts revision/time are numeric, not
strings; also guards the earlier `gobject_check` one-positional-
arg fix and the `getRawTransaction` verbose 0/1 mapping.

P2 (routes/govProposals.js): draft validation accepted arbitrarily
large digit strings / BigInts / numbers for `payment_amount_sats`
but never bounded them to SQLite INTEGER (signed 64-bit) range.
Oversized values overflowed the column on insert/update and
surfaced as a generic 500 instead of a deterministic 400. Added
an int64 ceiling via the existing `MAX_PAYMENT_AMOUNT_SATS`
constant in `proposalValidate.js`, applied after the shape
validation so all three accepted wire forms (BigInt / Number /
digit-string) flow through one gate and emit a new
`amount_too_large` issue code. Values are normalized to BigInt on
the way out so the drafts repo never re-parses a string.

Regression tests cover:
- proposalRpc: numeric vs string arg shape, gobject_check single
  arg, verbose mapping, upstream error passthrough.
- drafts create/patch: int64_max accepted, int64_max+1 rejected
  with amount_too_large, paymentAmount (decimal SYS) path also
  gated.

Made-with: Cursor
@sidhujag
Copy link
Copy Markdown
Member Author

@codex review. Don't worry about migrations, not an issue.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 30b58664f9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread routes/govProposals.js Outdated
Comment on lines +490 to +492
try {
rawForCanon.payment_amount_sats = BigInt(f.paymentAmountSats);
} catch {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reject unsafe numeric paymentAmountSats before hashing

POST /gov/proposals/prepare converts paymentAmountSats with BigInt(f.paymentAmountSats) when the client sends a JSON number, but JavaScript numbers above Number.MAX_SAFE_INTEGER are already rounded before this conversion. In that case the route silently commits a different amount than the caller sent, so the stored submission/canonical hash can encode the wrong payment amount without returning a validation error. Please reject non-safe numeric integers (or require string input for large values) before converting to BigInt.

Useful? React with 👍 / 👎.

…shing

Codex PR8 round 17 P2. Both POST /gov/proposals/prepare and the
drafts create/patch paths were running `BigInt(f.paymentAmountSats)`
(or accepting a `Number.isInteger(sats)` number) without bounding
numeric input to the JS safe-integer range. JSON.parse silently
rounds integers above `Number.MAX_SAFE_INTEGER (2^53 - 1)` at parse
time, so a caller that sent `9007199254740993` would see the server
canonicalize, hash, and persist `9007199254740992` — a silent
mismatch between the bytes the user typed and what the submission
row commits to.

Extract a shared `parsePaymentAmountSatsInput` helper in
routes/govProposals.js that accepts three wire shapes explicitly:
- BigInt: must be >= 0n, forwarded as-is.
- number: must be `Number.isSafeInteger` AND >= 0. Integers at or
  above 2^53 are rejected with a dedicated `amount_sats_unsafe_number`
  code so the frontend can direct the caller to resend as a digit
  string.
- string: must match `/^(0|[1-9][0-9]*)$/` — the canonical digit-
  only form. `BigInt` parses digit strings losslessly, so this is
  the recommended wire form for amounts above safe-integer.
The helper always returns a BigInt on success, which flows into
the existing int64 ceiling (MAX_PAYMENT_AMOUNT_SATS) gate without
further normalization.

Both /prepare and /drafts now route through the helper, replacing
the old `BigInt(...)` try/catch and the ad-hoc `Number.isInteger`
check that leaked rounded doubles through to persistence.

Regression tests cover:
- drafts create: unsafe JSON-number rejected with
  amount_sats_unsafe_number, Number.MAX_SAFE_INTEGER accepted,
  large digit-strings accepted.
- prepare: unsafe JSON-number rejected with the same code, large
  digit-strings pass end-to-end into a prepared submission.

Made-with: Cursor
@sidhujag
Copy link
Copy Markdown
Member Author

@codex review. Don't worry about migrations, not an issue.

@chatgpt-codex-connector
Copy link
Copy Markdown

Codex Review: Didn't find any major issues. More of your lovely PRs please.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@sidhujag sidhujag merged commit bcad33e into main Apr 22, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant